✅ Deploy Preview for nodemon ready!

Name	Link
🔨 Latest commit	65432a6
🔍 Latest deploy log	https://app.netlify.com/sites/nodemon/deploys/62bafb3c30e36400090079c9
😎 Deploy Preview	https://deploy-preview-2033--nodemon.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

this is good, but I'm wary your windows test didn't pass: https://github.com/alexbrazier/simple-update-notifier/runs/7033149767?check_suite_focus=true

this is good, but I'm wary your windows test didn't pass: https://github.com/alexbrazier/simple-update-notifier/runs/7033149767?check_suite_focus=true

@remy thanks for looking into it! I actually force pushed that update as the isTTY check was causing the update checker not to run on CI so had to update a flag to force it to run during the tests. That is original test run from the old commit and the latest one has all three passing https://github.com/alexbrazier/simple-update-notifier/actions/runs/2554631152

Edit: Looks like I was looking at the wrong build you referenced - that failure was an old one caused by the single quotes that Windows wasn't liking and has since been fixed

Seems it is not compatible with node 8.10.0 (minimum required version for nodemon) because for await
https://github.com/alexbrazier/simple-update-notifier/blob/91085738fbd27138dc510069d663e013e81af1eb/src/getDistVersion.ts#L10

Seems it is not compatible with node 8.10.0 (minimum required version for nodemon) because for await
https://github.com/alexbrazier/simple-update-notifier/blob/91085738fbd27138dc510069d663e013e81af1eb/src/getDistVersion.ts#L10

Thanks @zorn-v, I've now pushed an update to fix that

@alexbrazier worth wrapping this in a try/catch - https://github.com/alexbrazier/simple-update-notifier/blob/master/src/getDistVersion.ts#L13= - I can't remember what happens if this fails inside of the deep event handler - might be worth injecting some bad code to see how it behaves.

@alexbrazier worth wrapping this in a try/catch - https://github.com/alexbrazier/simple-update-notifier/blob/master/src/getDistVersion.ts#L13= - I can't remember what happens if this fails inside of the deep event handler - might be worth injecting some bad code to see how it behaves.

Sure, something like this? https://github.com/alexbrazier/simple-update-notifier/pull/2/files

Tbh @remy, you should probably choose this or #2035, because it is pretty bad to have a random vulnerability in a project, which is used by a lot of people. When using npm, the alert is kinda discouraging.

@alexbrazier have you thought about using semver module instead of implementing your own version check?

@alexbrazier have you thought about using semver module instead of implementing your own version check?

I was originally trying to keep it simple but now the version check has become more complicated than expected and still not perfect so happy to swap it out for semver if that is preferred?

Tbh @remy, you should probably choose this or #2035,

I'm letting @alexbrazier have some time to settle the code base. It's not a straight up simple change and a few extra days to help iron out issues will be worthwhile in the long run.

Plus, the reality of the vuln in nodemon has near zero impact. Of course it's not zero, but there's no path to exploit the actual vuln through nodemon (partly because it's fired outside of calling your code - which would take advantage of the vuln, and partly because nodemon in a lot of cases forks the sub process, so it never has access to any of the nodemon code - and thusly the vuln on got).

The issue isn't being ignored, @alexbrazier has kindly and valiantly taken up the mantle to solve this dependency once and for all.

Thanks @remy!

As the last couple of changes have been around some issues with the semver comparison and there were still some edge cases I've decided to just switch to using semver to make it more reliable. Should hopefully be good to go now unless anyone has any other comments.

semver 7.3.7 has

  "engines": {
    "node": ">=10"
  }

There is "semver": "^5.7.1" in nodemon deps

semver 7.3.7 has

  "engines": {
    "node": ">=10"
  }

There is "semver": "^5.7.1" in nodemon deps

Hmm, I've tested with node 8.10.0 and it seems to work as expected:

semver 7.3.7 has

  "engines": {
    "node": ">=10"
  }

There is "semver": "^5.7.1" in nodemon deps

Hmm, I've tested with node 8.10.0 and it seems to work as expected:

Are there any warnings during install if you're using node 8? It could be just that.

Are there any warnings during install if you're using node 8? It could be just that.

Nothing when installing with npm, but seems there is an issue with yarn

related to #2039 & #2040

Are there any warnings during install if you're using node 8? It could be just that.

Nothing when installing with npm, but seems there is an issue with yarn

You can use ~7.0.0 to avoid then